-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Fixed parser bug assuming value parsed is part of command #6636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #6636 +/- ##
===================================
Coverage 0% 0%
===================================
Files 11 11
Lines 145 145
Branches 11 11
===================================
Misses 145 145Continue to review full report at Codecov.
|
| value=value) | ||
| if not self.command_source: | ||
| # parser has no `command_source`, value is part of command itself | ||
| error_msg = "{prog}: '{value}' is not in the '{prog}' command-group. See '{prog} --help'.".format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to hyphenate command group.
|
Can we add a test for this? |
| telemetry.set_user_fault(error_msg) | ||
| logger.error(error_msg) | ||
| candidates = difflib.get_close_matches(value, action.choices, cutoff=0.8) | ||
| candidates = difflib.get_close_matches(value, action.choices, cutoff=0.7) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale behind this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I felt it a little too strict; 0.8 allows zero edits for words under 5 letters; and at most 1 edit until words of length 10 or more.
0.7 allows a edit for four-character words and 2 edits for words of length 7, 8, and 9.
This isn't the main purpose of this PR, however, and I can change this back if you guys prefer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with it--just wanted to know the reason.
|
@troydai I'll look into adding a test |
Added testing for spellchecker and msg choice
Fixes: #6562